Docs: Use non-SeqCst in module example of atomics #119138
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I done this for this reasons: 1. The example now shows that there is more Orderings than just SeqCst. 2. People who would copy from example would now have more suitable orderings for the job. 3. SeqCst is both much harder to reason about and not needed in most situations. IMHO, we should encourage people to think and use memory orderings that is suitable to task instead of blindly defaulting to SeqCst.
rustbot
added
S-waiting-on-review
| @@ -138,7 +138,7 @@ | |||
| //! | |||
| //! In general, *all* atomic accesses on read-only memory are Undefined Behavior. For instance, attempting | |||
| //! to do a `compare_exchange` that will definitely fail (making it conceptually a read-only | |||
| //! operation) can still cause a page fault if the underlying memory page is mapped read-only. Since | |||
| //! operation) can still cause a segmentation fault if the underlying memory page is mapped read-only. Since | |||
There was a problem hiding this comment.
AFAIK, page faults are about valid memory that is not mapped to physical pages and absolutely legal (but slow) while text describes illegal memory access which is called segmentation fault. I am not an expert though and may be not correct.
There was a problem hiding this comment.
I think page fault is the more precise term here it's more lower level. The OS will then either do something useful with the page fault and continue normally, or it will crash the process with a SIGSEGV if it did something bad (which would be the case here) (I'm not entirely sure whether this is accurate but I think it is?). I'm not sure what the best way to describe this is here, it seems fine to change it to segmentation fault, that is probably more useful to users.
|
r? libs |
|
r? libs |
Noratrieb
approved these changes
Jan 18, 2024
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 19, 2024
…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#118665 (Consolidate all associated items on the NonZero integer types into a single impl block per type) - rust-lang#118798 (Use AtomicU8 instead of AtomicUsize in backtrace.rs) - rust-lang#119062 (Deny braced macro invocations in let-else) - rust-lang#119138 (Docs: Use non-SeqCst in module example of atomics) - rust-lang#119907 (Update `fn()` trait implementation docs) - rust-lang#120083 (Warn when not having a profiler runtime means that coverage tests won't be run/blessed) - rust-lang#120107 (dead_code treats #[repr(transparent)] the same as #[repr(C)]) - rust-lang#120110 (Update documentation for Vec::into_boxed_slice to be more clear about excess capacity) - rust-lang#120113 (Remove myself from review rotation) - rust-lang#120118 (Fix typo in documentation in base.rs) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 19, 2024
…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#118665 (Consolidate all associated items on the NonZero integer types into a single impl block per type) - rust-lang#118798 (Use AtomicU8 instead of AtomicUsize in backtrace.rs) - rust-lang#119062 (Deny braced macro invocations in let-else) - rust-lang#119138 (Docs: Use non-SeqCst in module example of atomics) - rust-lang#119907 (Update `fn()` trait implementation docs) - rust-lang#120083 (Warn when not having a profiler runtime means that coverage tests won't be run/blessed) - rust-lang#120107 (dead_code treats #[repr(transparent)] the same as #[repr(C)]) - rust-lang#120110 (Update documentation for Vec::into_boxed_slice to be more clear about excess capacity) - rust-lang#120113 (Remove myself from review rotation) - rust-lang#120118 (Fix typo in documentation in base.rs) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 19, 2024
Rollup merge of rust-lang#119138 - AngelicosPhosphoros:use_proper_atomics_in_spinlock_example, r=Nilstrieb Docs: Use non-SeqCst in module example of atomics I done this for this reasons: 1. The example now shows that there is more Orderings than just SeqCst. 2. People who would copy from example would now have more suitable orderings for the job. 3. SeqCst is both much harder to reason about and not needed in most situations. IMHO, we should encourage people to think and use memory orderings that is suitable to task instead of blindly defaulting to SeqCst. r? `@m-ou-se`
AngelicosPhosphoros
deleted the
use_proper_atomics_in_spinlock_example
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I done this for this reasons:
IMHO, we should encourage people to think and use memory orderings that is suitable to task instead of blindly defaulting to SeqCst.
r? @m-ou-se